Fix attribute array comparison#6181
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6181 +/- ##
==========================================
- Coverage 89.83% 89.83% -0.01%
==========================================
Files 88 88
Lines 23211 23208 -3
Branches 4316 4316
==========================================
- Hits 20852 20849 -3
Misses 1627 1627
Partials 732 732 ☔ View full report in Codecov by Sentry. |
|
Note, this change fixes what I believe is a bug here: Lines 388 to 391 in d3071ff Line 391 should be checking for any differences in the array elements, not all. It also cleans up a now unnecessary Lines 108 to 111 in d3071ff I have extended the existing unit test to check for correct handling inequality. iris/lib/iris/tests/unit/common/mixin/test_LimitedAttributeDict.py Lines 45 to 49 in d3071ff which now fails as it is testing a scalar value against a single element array. With the old logic, this would work, but it is flawed as np.all( np.array([0]) == 0 ) and np.all( np.zeros(100) == 0 ) would both return True, even though the latter is not what is intended.
I have commented out this test at the moment and would appreciate the opinion of @pp-mo. |
…bunney/iris into bugfix/cube_attr_compare * 'bugfix/cube_attr_compare' of https://github.com/ukmo-ccbunney/iris: [pre-commit.ci] auto fixes from pre-commit.com hooks
pp-mo
left a comment
pp-mo
left a comment
There was a problem hiding this comment.
See comment where I think this changes too much from as-is behaviour.
Otherwise all good , I think
…mpare * upstream/main: Document use of new_axis to control merge (SciTools#6180) Updated environment lockfiles (SciTools#6184) [pre-commit.ci] pre-commit autoupdate (SciTools#6175) Updated environment lockfiles (SciTools#6183)
ESadek-MO
left a comment
ESadek-MO
left a comment
There was a problem hiding this comment.
Happy to build on top of @pp-mo 's review and consider this good to go!
Thanks @ukmo-ccbunney !
ESadek-MO
dismissed
pp-mo’s stale review
@pp-mo is currently unavailable to approve change
* upstream/main: (194 commits) Restore latest Whats New files. [pre-commit.ci] pre-commit autoupdate (SciTools#6205) correct major minor in whatsnew (SciTools#6202) What's new updates for v3.11.0rc0 . (SciTools#6201) Update CF standard names table. (SciTools#6200) Specify meta in dask.array.map_blocks (SciTools#5989) added in a vertical rule for surface fields (SciTools#5734) Updated environment lockfiles (SciTools#6197) Reduce duplication in cf.py spanning checks. (SciTools#6196) Fix attribute array comparison (SciTools#6181) Enable factory references to create new dimensions on load. (SciTools#6168) (SciTools#6194) Improve handling of masks in concatenate (SciTools#6187) Demo Numpy v2 (SciTools#6035) Bump scitools/workflows from 2024.10.1 to 2024.10.2 (SciTools#6186) Document use of new_axis to control merge (SciTools#6180) Updated environment lockfiles (SciTools#6184) [pre-commit.ci] pre-commit autoupdate (SciTools#6175) Updated environment lockfiles (SciTools#6183) Update to `cell_method` parsing (SciTools#6083) Bump scitools/workflows from 2024.10.0 to 2024.10.1 (SciTools#6179) ...
3 participants
🚀 Pull Request
Description
Fixes: #6027
Logic for comparing arrays in attributes was relying on old numpy behaviour where an array compared to another any object would return a scalar
TrueorFalse. Current numpy behaviour is to do a element-wise comparison which throws an error if the arrays are of different size.Fix is to use the
numpy.array_equal.Consult Iris pull request check list